fix(workflow-executor): harden OAuth reauth and token write-back [PRD-692]#1724
Conversation
|
Coverage Impact This PR will not change total coverage. Modified Files with Diff Coverage (6) 🛟 Help
|
Three correctness fixes that gate the OAuth2 MCP executor flag: - clear the step idempotency marker on a re-auth pause so a resumed step is no longer rejected as interrupted - record a re-auth pause as a non-failure in the activity log instead of a spurious failure - re-check credential existence before the rotated-token write-back so a concurrent disconnect is not silently resurrected Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Assert the activity entry is closed as succeeded on a re-auth pause, not just that it was never failed. Drop the stale TDD/iteration framing and ticket references from the re-auth pause test block. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replace the get-then-upsert existence re-check with an atomic updateIfPresent (UPDATE ... WHERE) on the credentials store, closing the read-to-write window where a concurrent disconnect could be resurrected. upsert stays for the consent deposit path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Revert the isNonFailure audit special-casing on the re-auth pause path. An MCP tool call that 401s has genuinely failed, so it should surface as a failed audit entry (useful for observability) rather than be recorded as completed. The step still pauses (awaiting-input) and the resumed run logs its own entry. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
26e0158 to
3afe056
Compare
…cope Addresses review findings on the re-auth pause and refresh write-back: - preserve a confirmation-flow step's approved pendingData on a re-auth pause (clear only the marker) so resume replays that exact call; delete only when there is no pendingData (FullyAutomated), which would otherwise mis-route the resumed step into the confirmation flow - make the pause cleanup best-effort so a store error still returns awaiting-input instead of a hard failure - key updateIfPresent on the row id so a disconnect + re-authorize is not clobbered by a stale in-flight write-back (a re-created row has a new id) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Keep the non-obvious why (preserve-vs-delete on re-auth pause, id-scoped write-back); drop what the method names and code already state. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Swallowing a cleanup failure left the 'executing' marker in place, so the pause returned awaiting-input but could never resume (checkIdempotency rejects the stale marker). Let the store error propagate to an ordinary step error instead — consistent with the executor's other store-failure paths. Supersedes the earlier best-effort try/catch. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Scra3
left a comment
There was a problem hiding this comment.
praise: the clearReauthPauseState fix correctly clears the write-ahead 'executing' marker on the tool-call re-auth-pause path (delete for FullyAutomated, keep pendingData for confirmation flows), with resume-to-success and cleanup-failure coverage; the updateIfPresent id-scoped write-back also correctly stops a concurrent disconnect from resurrecting a deleted credential. No issues in this PR's diff. Heads-up: the SSRF blocker for this feature lives in the base branch (#1665, oauth/refresh-grant.ts — fetch follows redirects), and the evict/set race is also base-branch code.
9d48e40
into
feat/prd-367-pr2-executor-oauth-runtime
…PRD-367] (#1665) * feat(workflow-executor): use stored OAuth credentials for MCP steps At an oauth2 MCP step the executor looks up the stored credential by (user, server), runs the refresh-token grant against the stored token endpoint behind an expiry-skew cache, injects the bearer token before connecting, retries once on a 401 across list-tools and the tool call, and pauses for re-authentication when no usable credential exists or the refresh is rejected. Bearer and none steps are unchanged. Adds additive auth-error classification to the shared ai-proxy McpClient consumed by this path. Behaviour stays dormant until the orchestrator serves authType and the frontend ships (deploy orchestrator first), so it is safe to deploy alone. Depends on the PR1 credential store. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(workflow-executor): write rotated refresh token to the current row On the invalid_grant concurrent-rotation retry path the write-back used the pre-retry credential for the non-token fields; thread the credential whose token produced the grant through so a concurrent re-deposit is not partially reverted. Addresses review on #1665. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(ai-proxy): treat only 401 (not 403) as a refreshable MCP auth error A 403 is a permission/scope failure that a token refresh or re-consent cannot resolve, so it no longer triggers the refresh + re-auth flow (which looped) and instead surfaces as an ordinary failure. The spec specifies retry on 401. Addresses review on #1665. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(workflow-executor): support OAuth MCP steps in the in-memory executor [PRD-367] PR1 wired an in-memory credential store + deposit endpoint into buildInMemoryExecutor, so the previous "in-memory raises ConfigurationError for oauth2 steps" behavior was inconsistent: a credential could be deposited but never used. Wire an OAuthTokenService into the in-memory runner (sharing the same store instance the deposit endpoint writes to) so oauth2 steps work end-to-end in dev, matching the database executor. The token service is now a required RunnerConfig/RemoteToolFetcher collaborator (both executors provide it), so the unreachable ConfigurationError guard and its fetcher test are removed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(workflow-executor): repoint token service at the relocated credentials port [PRD-367] The PR1 rebase moved the credentials store interface + types from stores/mcp-oauth-credentials-store to ports/mcp-oauth-credentials-store (the store file now holds only the Database/InMemory implementations). Import McpOAuthCredentialsStore and StoredMcpOAuthCredential from the new port path so the package compiles against the rebased PR1 base. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(workflow-executor): re-consent on decrypt failure after key rotation [PRD-367] PR1 dropped enc_key_version from the credential store (no version-aware decrypt path), so the rotation write-back no longer carries encKeyVersion. Per PRD-367 key-rotation handling, a decrypt failure with the encryption key PRESENT (auth-tag mismatch from a since-rotated/hard-swapped key) is recoverable: toGrantParams now classifies it as OAuthReauthRequiredError (needs-oauth-reauth) so re-consent re-deposits under the new key. A missing key (ExecutorEncryptionKeyMissingError) stays terminal — re-consent cannot help and a re-deposit would 503. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(workflow-executor): add GET /list-mcp-tools design-time tool listing [PRD-367] New GET /list-mcp-tools?mcpServerId= route on the executor HTTP server for the orchestrator-engine MCP-server details page: resolves the caller's vault credential (user_id from the validated JWT, never the request), refreshes, injects the Bearer, and returns the server's tool definitions — reusing RemoteToolFetcher, no new fetch/refresh logic. A missing/unrefreshable credential returns a typed needs-oauth-reauth (409), not a generic error or empty list. Wired into both the database and in-memory executors so oauth2 tool listing works in dev too. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(workflow-executor): guard against non-object token-endpoint responses [PRD-367] A literal JSON null (or other non-object) body from the token endpoint overwrote the {} parse default, so the subsequent payload.error / payload.access_token reads threw a TypeError instead of the typed OAuthRefreshError. Keep the {} default for non-object bodies so the status checks still surface a typed OAuthRefreshError. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(workflow-executor): evict cached access token on credential disconnect [PRD-367] On DELETE /mcp-oauth-credentials/:mcpServerId, evict the in-process cached access token for (user, server) so a disconnect takes effect immediately — otherwise the executor keeps serving the cached token until it expires even though the credential row is gone (surfaced by end-to-end testing). Wires OAuthTokenService into both executor builders + the HTTP server (optional on the options so credential-free tests need not construct one), and adds OAuthTokenService.evict with coverage. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(workflow-executor): address PR review on oauth2 runtime [PRD-624] - list-mcp-tools: map ExecutorEncryptionKeyMissingError to the typed 503 { code } the deposit endpoint already returns, so the details page shows the admin message instead of a generic error. - token-service: encrypt the rotated refresh token inside the best-effort write-back try, so an encrypt failure can't fail an otherwise-valid getAccessToken. - ai-proxy isMcpAuthError: an explicit status is authoritative — a 403 is not refreshable even when its message says "unauthorized"; fall back to the message only when no status is present. - Trim comments to why-not-how. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(workflow-executor): correct write-back-failure comment [PRD-624] The re-read recovery path only handles a peer instance's rotation, not our own failed write-back (the stored token is unchanged, so re-read sees no rotation and forces re-auth). Describe the actual fallback: re-authentication. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(workflow-executor): validate OAuth token endpoint against SSRF [PRD-624] The token endpoint is where the executor POSTs the refresh grant (with client credentials), but it was stored as an unconstrained string, so an authenticated caller could aim the executor at an internal address (SSRF, incl. cloud metadata). Add assertSafeTokenEndpoint, enforced at deposit (400) and again before the refresh POST (defence in depth for pre-existing rows): - scheme must be http(s); https required in production (http stays allowed off-prod for the local OAuth sim); - link-local / cloud-metadata (169.254.0.0/16, fe80::/10) blocked everywhere; - loopback blocked in production; - RFC1918 private ranges stay allowed — private OAuth providers are supported. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(workflow-executor): block IPv4-mapped IPv6 in token-endpoint guard [PRD-624] The URL parser normalizes `::ffff:127.0.0.1` to the hex form `::ffff:7f00:1`, which the IPv6 branch classified as neither loopback nor link-local — so a mapped loopback/metadata address slipped past the SSRF block in production. Extract the embedded IPv4 from `::ffff:` addresses and run it through the IPv4 loopback/link-local checks. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(workflow-executor): use US spelling "defense" in refresh-grant comment [PRD-624] Match the codebase convention (run-to-available-step-mapper.ts). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(workflow-executor): rewrite token-endpoint guard comment [PRD-624] Drop the local-test-tool and ticket references; explain the why (SSRF rationale, the deliberate RFC1918 allowance, why no DNS resolution) rather than enumerating the rules. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(workflow-executor): block the unspecified address in token-endpoint guard [PRD-624] 0.0.0.0 (and ::) connects to loopback on Linux, so it was an SSRF bypass the guard accepted. Classify the unspecified range (0.0.0.0/8, ::) and reject it on every environment — it is never a valid token endpoint. Surfaced by evaluating ipaddr.js, which flags it as `unspecified`; the WHATWG URL parser already normalizes the decimal/hex/short-form IP encodings to dotted form, so those were already covered. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(workflow-executor): reject localhost FQDN/.localhost aliases in token guard [PRD-624] Only the bare `localhost` was treated as loopback, so `localhost.` and the `.localhost` TLD (RFC 6761) — both resolving to loopback — slipped past the guard in production. Match any host whose last label is `localhost` (with or without a trailing dot), without over-blocking real domains like auth.localhost.example.com. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(workflow-executor): cover trailing-dot IP forms in token-endpoint guard [PRD-624] Regression test proving `127.0.0.1.` / `169.254.169.254.` are rejected: the guard runs `new URL()` first, which normalizes the trailing dot before classification, so the trailing-dot form is not a bypass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(workflow-executor): address PR review — SSRF redirect, deposit evict, fail-closed env [PRD-624] - refresh-grant: reject redirects on the token POST (redirect:'manual' + any 3xx → OAuthRefreshError) so a validated token endpoint can't 3xx the grant body (refresh token / client secret) to an internal host, bypassing the endpoint validation. - executor-http-server: evict the cached access token on deposit too, so a reconnect / re-deposit takes effect immediately instead of serving the prior token until expiry (mirrors the delete path). - token-endpoint-url: fail closed — the strict https/no-loopback rules now apply unless NODE_ENV explicitly opts into the dev/test relaxation, so an unset or misspelled NODE_ENV can no longer silently allow loopback targets or cleartext http. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(workflow-executor): harden OAuth reauth and token write-back [PRD-692] (#1724) * fix(workflow-executor): harden OAuth reauth and token write-back Three correctness fixes that gate the OAuth2 MCP executor flag: - clear the step idempotency marker on a re-auth pause so a resumed step is no longer rejected as interrupted - record a re-auth pause as a non-failure in the activity log instead of a spurious failure - re-check credential existence before the rotated-token write-back so a concurrent disconnect is not silently resurrected Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(workflow-executor): strengthen reauth-pause audit assertion Assert the activity entry is closed as succeeded on a re-auth pause, not just that it was never failed. Drop the stale TDD/iteration framing and ticket references from the re-auth pause test block. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(workflow-executor): make OAuth refresh write-back atomic Replace the get-then-upsert existence re-check with an atomic updateIfPresent (UPDATE ... WHERE) on the credentials store, closing the read-to-write window where a concurrent disconnect could be resurrected. upsert stays for the consent deposit path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(workflow-executor): audit a re-auth-paused MCP call as failed Revert the isNonFailure audit special-casing on the re-auth pause path. An MCP tool call that 401s has genuinely failed, so it should surface as a failed audit entry (useful for observability) rather than be recorded as completed. The step still pauses (awaiting-input) and the resumed run logs its own entry. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(workflow-executor): correct reauth-pause cleanup and write-back scope Addresses review findings on the re-auth pause and refresh write-back: - preserve a confirmation-flow step's approved pendingData on a re-auth pause (clear only the marker) so resume replays that exact call; delete only when there is no pendingData (FullyAutomated), which would otherwise mis-route the resumed step into the confirmation flow - make the pause cleanup best-effort so a store error still returns awaiting-input instead of a hard failure - key updateIfPresent on the row id so a disconnect + re-authorize is not clobbered by a stale in-flight write-back (a re-created row has a new id) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * style(workflow-executor): trim added comments to the 2-line convention Keep the non-obvious why (preserve-vs-delete on re-auth pause, id-scoped write-back); drop what the method names and code already state. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(workflow-executor): propagate reauth-cleanup store errors Swallowing a cleanup failure left the 'executing' marker in place, so the pause returned awaiting-input but could never resume (checkIdempotency rejects the stale marker). Let the store error propagate to an ordinary step error instead — consistent with the executor's other store-failure paths. Supersedes the earlier best-effort try/catch. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(workflow-executor): link the OAuth2-MCP test server Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

What
Two correctness fixes on the executor OAuth2 MCP runtime, surfaced in the PR2 (#1665) review and deliberately deferred to land before the feature is enabled (PRD-627). Both touch the store-port surface, so they're bundled.
idempotencyPhase: 'executing'(HIGH) — a reauth pause left the write-ahead marker set, so the resumed step was rejected with "Step execution was interrupted". AddeddeleteStepExecution(runId, stepIndex)to theRunStoreport (both store impls) and clear the marker on theOAuthReauthRequiredErrorpath.updateIfPresent(UPDATE … WHERE) on the credentials store instead of get-then-upsert, so a concurrent disconnect (DELETE) cannot be resurrected.upsertstays for the consent deposit path.Audit-log behavior (intentional)
A re-auth pause's MCP tool call genuinely 401'd, so it surfaces as a
failedactivity-log entry. The audit records the call (which failed); the run history records the step (which pauses withneeds-oauth-reauthand resumes). PR2's original "spurious failure" finding was reconsidered and dropped: the failure is real and worth surfacing for observability, and the resumed run logs its own entry — so no audit special-casing. (The earlierpaused-status follow-up, PRD-693, is cancelled for the same reason.)Stacking
Stacked on PR2 (#1665) — this PR targets
feat/prd-367-pr2-executor-oauth-runtime, notmain, because the OAuth runtime code it fixes only exists on that branch. Once PR2 merges, rebase and repoint this PR tomain.Tests
failedwhile the step pauses, and the write-back race (a concurrently-deleted credential is not resurrected).deleteStepExecutioncovered in both run stores;updateIfPresentcovered in both credential stores (updates in place / does not insert when absent).tsc+ eslint clean. Postgres*.pg.test.tsleft to CI.fixes PRD-692
🤖 Generated with Claude Code
Note
Harden OAuth re-auth pause state and rotated refresh token write-back in
McpStepExecutorMcpStepExecutor.doExecutenow calls a newclearReauthPauseState()helper before returningawaiting-input, preventing later resumes from being rejected as interrupted due to a staleexecutingwrite-ahead marker.clearReauthPauseStateclears only theidempotencyPhasewhenpendingDataexists (preserving confirmation-flow state), or deletes the step execution record entirely when there is nopendingData.OAuthTokenService.persistRotatedRefreshTokennow callsstore.updateIfPresent(id, ...)instead ofstore.upsert(...), making rotated token write-backs a no-op if the credential row was deleted or replaced since the token was read.McpOAuthCredentialsStoreandRunStoreinterfaces gain new methods (updateIfPresentanddeleteStepExecution) implemented across the database and in-memory stores.Macroscope summarized 50b58c5.